-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(auth): add support for function for password #2039
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
* origin/main: chore(release): 5.8.2 [skip ci] fix: move CLIENT SETINFO commands to connection handshake (redis#2033) ci(test): add redis matrix and update docker compose file (redis#2034) docs(example): add example for setting TTL to "HSET Field" (redis#2027) fix: default IP family selection to 0 (redis#2028) chore(release): 5.8.1 [skip ci] test: add scenario tests v5 (redis#2020) fix(ssubscribe): re-subscribe sharded pubsub channels individually (redis#2021) chore(release): 5.8.0 [skip ci] feat: support client setinfo (redis#2011) feat(stream): Add XDELEX command (redis#2003) Add stale issue management workflow (redis#2018) feat: implement proper hpexpire command signatures and tests (redis#2006) feat: add more xtrim method overloads and tests (redis#2010) test(cluster): fix and add cluster tests in CI (redis#2017) Force slots refresh on MOVED error when using ssubscribe (redis#2013) fix(ssubscribe): re-subscribe sharded pubsub channels individually on ready (redis#2012) chore(release): 5.7.0 [skip ci] fix: xread example for TypeScript (redis#1872) feat: Implement hexpire for redis#1898 (redis#1918)
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
| : options.password, | ||
| subscriber: false, | ||
| }; | ||
| this.resolvePassword((err, resolvedPassword) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big change from @slukes work. Rather than use an async promise here, the resolvePassword logic tries to keep everything synchronous when it can. It's only when this.options.password returns a promise that this callback will be invoked in a separate tick.
If this is changed into async logic, there are a few tests that fail because there seems to be an expectation that the connect logic runs the connector which sets this.connecting = true before any other code runs.
e.g., the following test would fail when making resolvePassword async:
it("connects successfully immediately after end", (done) => {
const redis = new Redis();
redis.once("end", async () => {
await redis.connect();
done();
});
redis.quit();
});The connect function (which is called from the constructor) calls into the connector and sets connecting to true on the connector. If that variable is no longer true in the next tick, it rejects the connect request and will trigger a transition into the end state.
However, if connect needs to resolve something else asynchronously, by the time redis.quit runs, the connector may not have run yet. The net effect of this is that the client will first disconnect and then re-connect, because the end-user has no control over the connect.
All of that said, this test could be changed to wait until the client is ready. Would that be breaking behaviour?
it("connects successfully immediately after end", (done) => {
const redis = new Redis();
redis.once("end", async () => {
await redis.connect();
done();
});
redis.on("ready", () => redis.quit());
});There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Jit has detected 1 important finding in this PR that you should review.
The finding is detailed below as a comment.
It’s highly recommended that you fix this security issue before merge.
Repository Risks:
- Database Integration: Connects to a database, often involving sensitive data that must be securely managed.
- Critical Severity Findings: Indicates that the resource has critical severity security findings that need immediate action.
- Internally Accessible: Accessible only within the internal network, reducing exposure to external threats but still requiring proper controls.
Repository Context:
graph LR
GitHub$Repository_U23_redis/ioredis["GitHub Repository<br/>redis/ioredis"]:::GitHub$Repository
Team_U23_client_U2D_developers["Team<br/>client-developers"]:::Team
DBIntegration_U23_redis["DBIntegration<br/>redis"]:::DBIntegration
Team_U23_client_U2D_developers -- "Owns" --> GitHub$Repository_U23_redis/ioredis
GitHub$Repository_U23_redis/ioredis -- "Is accessible to" --> DBIntegration_U23_redis
| // Note that `this.condition` has to be set _before_ any asynchronous work | ||
| // takes place as the `select` value is required when queueing commands | ||
| // into the offline queue (see sendCommand) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another change from @slukes work. This allows pipelining and the offline queue to function.
|
|
||
| this.connectionEpoch += 1; | ||
| this.setStatus("connecting"); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tip: This PR becomes a lot more reviewable when hiding whitespace changes ...
* origin/main: fix: remove unnecessary case-sensitivity when working with commands (redis#2036) ci: update redis test images (redis#2035)
|
Hey @simong thank you for creating this PR, I'm going to take a look at it later |
|
run sharded pub sub tests |
|
Builds on #1985 and fixes the test failures.
PRing mainly to get feedback.